Skip to content

Builtin submodules#1321

Merged
rocky merged 5 commits intomasterfrom
builtin-submodules
May 1, 2021
Merged

Builtin submodules#1321
rocky merged 5 commits intomasterfrom
builtin-submodules

Conversation

@rocky
Copy link
Copy Markdown
Member

@rocky rocky commented Apr 26, 2021

While there is more that could be done in terms of breaking out subcategories of builtins, and more
cleanup in the ugly builtins/__init__.py, I would like to leave that for later.

There there are a lot of changes here that I'd like to get in to set a baseline for further work.

Also for later is hooking the new sectioning into the doc system so that it is more structured as well.

With this, I feel better about doubling the size of the builtins which is what we realistically need to cover all of WL.

@rocky rocky requested a review from mmatera April 26, 2021 20:17
@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 26, 2021

@mmatera Given the problems. I may just narrow this to basically the first commit. And then go in again and do again, etc.

@rocky rocky marked this pull request as draft April 26, 2021 20:54
@rocky rocky force-pushed the builtin-submodules branch 8 times, most recently from 6fc12ca to f13f061 Compare April 29, 2021 07:11
@rocky rocky marked this pull request as ready for review April 29, 2021 12:39
@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 29, 2021

@mmatera this is the next step in the progression. Modules that are to be cythonized for now remain in the builtin directory until we can adjust setup.py accordingly. However I want to leave that for some other time.

test_get_and_put() in test/test_files.py still has some OS specific behavior so that has been disabled on win32. That too feels like a totally separate problem.

@rocky rocky force-pushed the builtin-submodules branch 2 times, most recently from 33f69f4 to ef6fb51 Compare April 29, 2021 12:48
@mmatera
Copy link
Copy Markdown
Contributor

mmatera commented Apr 29, 2021

@mmatera this is the next step in the progression. Modules that are to be cythonized for now remain in the builtin directory until we can adjust setup.py accordingly. However I want to leave that for some other time.

test_get_and_put() in test/test_files.py still has some OS specific behavior so that has been disabled on win32. That too feels like a totally separate problem.

Just in case, both in MS Windows and Linux, WMA accepts filenames with "/" as the path separator. I think that Mathics does too.

@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 29, 2021

@mmatera this is the next step in the progression. Modules that are to be cythonized for now remain in the builtin directory until we can adjust setup.py accordingly. However I want to leave that for some other time.
test_get_and_put() in test/test_files.py still has some OS specific behavior so that has been disabled on win32. That too feels like a totally separate problem.

Just in case, both in MS Windows and Linux, WMA accepts filenames with "/" as the path separator. I think that Mathics does too.

I don't have Windows running right now, but that wasn't the problem. I think it had to do with assumptions about TemporaryDirectory or the kind of object returned.

At some point we should make a dedicated effort to fixing up test/test_files.py

@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 29, 2021

@mmatera this is the next step in the progression. Modules that are to be cythonized for now remain in the builtin directory until we can adjust setup.py accordingly. However I want to leave that for some other time.
test_get_and_put() in test/test_files.py still has some OS specific behavior so that has been disabled on win32. That too feels like a totally separate problem.

Just in case, both in MS Windows and Linux, WMA accepts filenames with "/" as the path separator. I think that Mathics does too.

In addition to what I wrote above, it might not have anything to do with the test and its expectation but instead it may be that we have implemented this in a way that doesn't work on all variants of Windows or Windows with Anaconda and so on. I probably should have investigated further but I didn't.

@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 30, 2021

@mmatera this is the next step in the progression. Modules that are to be cythonized for now remain in the builtin directory until we can adjust setup.py accordingly. However I want to leave that for some other time.
test_get_and_put() in test/test_files.py still has some OS specific behavior so that has been disabled on win32. That too feels like a totally separate problem.

Just in case, both in MS Windows and Linux, WMA accepts filenames with "/" as the path separator. I think that Mathics does too.

I fired up Windows to get a more precise answer to this than I gave before. Here is what I get when I try running the test in an anaconda shell

(base) C:\cygwin64\home\rocky\src\external-vcs\github\mathics\Mathics>py.test test\test_files.py

(base) C:\cygwin64\home\rocky\src\external-vcs\github\mathics\Mathics>pytest test\test_files.py
================================================= test session starts =================================================
platform win32 -- Python 3.8.5, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: C:\cygwin64\home\rocky\src\external-vcs\github\mathics\Mathics
collected 3 items

test\test_files.py ..F                                                                                           [100%]

====================================================== FAILURES =======================================================
__________________________________________________ test_get_and_put ___________________________________________________

    def test_get_and_put():
        temp_filename = evaluate('$TemporaryDirectory<>"/testfile"').to_python()
        temp_filename_strip = temp_filename[1:-1]
>       check_evaluation(f"40! >> {temp_filename_strip}", "Null")

test\test_files.py:26:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

str_expr = 'ToString[40! >> C:\\Users\\rocky\\AppData\\Local\\Temp/testfile]', str_expected = 'ToString[Null]'
message = '', to_string_expr = True, to_string_expected = True

    def check_evaluation(
        str_expr: str,
        str_expected: str,
        message="",
        to_string_expr=True,
        to_string_expected=True,
    ):
        """Helper function to test Mathics expression against
        its results"""
        if to_string_expr:
            str_expr = f"ToString[{str_expr}]"
            result = evaluate_value(str_expr)
        else:
            result = evaluate(str_expr)

        if to_string_expected:
            str_expected = f"ToString[{str_expected}]"
            expected = evaluate_value(str_expected)
        else:
            expected = evaluate(str_expr)

        print(time.asctime())
        if message:
            print((result, expected))
            assert result == expected, message
        else:
            print((result, expected))
>           assert result == expected
E           AssertionError

test\helper.py:40: AssertionError
------------------------------------------------ Captured stdout call -------------------------------------------------
Fri Apr 30 09:01:00 2021
('815915283247897734345611269596115894272000000000 >> C:\\\\Users\rocky\\\\AppData\\\\Local\\\\Temp/testfile', 'Null')

@mmatera
Copy link
Copy Markdown
Contributor

mmatera commented Apr 30, 2021

Thanks a lot for doing this. So, OK, I see the problem: the output uses the system path separator. I can fix this test later then.

@mmatera
Copy link
Copy Markdown
Contributor

mmatera commented Apr 30, 2021

Regarding this PR: it looks great, but I am worried about how disrupting would be to merge this over the other pending PRs. If we can merge them without many complications, then let's merge this.

@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 30, 2021

Regarding this PR: it looks great, but I am worried about how disrupting would be to merge this over the other pending PRs. If we can merge them without many complications, then let's merge this.

Valid concern and I am glad you bring this up. What I will do when I have a chance over the weekend is add PR's for the pending PR (other than the one that needs to be broken up anyway) that are based on this PR. This is before merging so that we can see and determine what's up.

For the one that need breaking up, I can add test locally and add PRs for the pieces.

@rocky
Copy link
Copy Markdown
Member Author

rocky commented Apr 30, 2021

So in sum, expect a number of small PR's, some of which haven't happened yet.

@rocky rocky force-pushed the builtin-submodules branch from ef6fb51 to 68f24bd Compare May 1, 2021 04:55
@rocky rocky merged commit f2597b9 into master May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants